Skip to content

Conversation

tysonthomas9
Copy link
Collaborator

@tysonthomas9 tysonthomas9 commented Sep 6, 2025

  • Update agent framework structure
  • Add more tests

Summary by CodeRabbit

  • New Features

    • Rich AI Chat UI: message list, input bar, model selector, tool call/result cards, agent session header, live nested session timeline, OAuth connect panel, version banner, and copy/feedback actions.
  • Improvements

    • Structured-response viewer with auto-open and “View Full Report”.
    • Better markdown rendering (TOC, CSS handling) and centralized scroll/auto-size behavior.
    • Vision-aware message/tool sanitization with clear "Image omitted" placeholders.
    • Real-time agent progress events and richer session/tool updates.
  • Bug Fixes

    • Normalized tool-call payloads for Groq integrations; input clears after send.

Copy link

coderabbitai bot commented Sep 6, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds provider-aware agent runtime, capability-aware message sanitization, an event bus, expanded LLM-context tooling, a large set of new AI-chat UI web components and renderers, shared UI-agnostic chat types, many tests, build/bundling updates, and multiple import/signature adjustments to propagate LLM provider/vision context.

Changes

Cohort / File(s) Summary
Build & bundling
config/gni/devtools_grd_files.gni, front_end/BUILD.gn, front_end/panels/ai_chat/BUILD.gn
Expanded bundled/unbundled manifests and public_deps to include many new ai_chat sources, metadata, and tests.
Shared chat types
front_end/panels/ai_chat/models/ChatTypes.ts
New UI-agnostic chat enums/interfaces/types for messages, tool results, agent sessions, image inputs, and state.
Scroll helper & ai_assistance
front_end/panels/ai_assistance/components/ScrollPinHelper.ts, front_end/panels/ai_assistance/components/ChatView.ts, front_end/panels/ai_assistance/BUILD.gn
Adds ScrollPinHelper; ChatView replaced manual scroll logic with helper; ImageInputData type expanded; BUILD.gn updated to include helper.
LLM sanitization & Groq
front_end/panels/ai_chat/LLM/MessageSanitizer.ts, .../LLM/__tests__/MessageSanitizer.test.ts, front_end/panels/ai_chat/LLM/GroqProvider.ts
New sanitizeMessagesForModel with options and tests; GroqProvider normalizes tool_calls/content and defaults tool_choice to 'auto' when tools present.
Agent runner & event bus
front_end/panels/ai_chat/agent_framework/AgentRunner.ts, .../AgentRunnerEventBus.ts, .../__tests__/AgentRunner.*.test.ts, .../ConfigurableAgentTool.ts
AgentRunner made provider-driven, adds getVisionCapability plumbing, message sanitization usage, nested sessions and overrides, computeToolResultText, event emissions; new event bus singleton and related tests; ConfigurableAgentTool.execute accepts ctx.
Agent graph & service
front_end/panels/ai_chat/core/{Graph.ts,ConfigurableGraph.ts,AgentNodes.ts,AgentService.ts}
Graph and node signatures now accept LLMProvider/provider; AgentNodes inject provider; AgentService subscribes to event bus, tracks active sessions, and exposes new session/tool events and getActiveAgentSessions().
Tooling: LLMContext & signatures
front_end/panels/ai_chat/tools/... (many files, e.g., Tools.ts, ThinkingTool.ts, SequentialThinkingTool.ts, SchemaBasedExtractorTool.ts, HTMLToMarkdownTool.ts, FetcherTool.ts, CombinedExtractionTool.ts, StreamlinedSchemaExtractorTool.ts, CritiqueTool.ts, FullPageAccessibilityTreeToMarkdownTool.ts)
Introduces LLMContext/LLMProvider types; updates Tool.execute signatures to accept optional ctx; tools propagate provider/model/vision, add context validation, retries, and error paths; new ObjectiveDrivenActionTool added.
Agent framework import path moves
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts
Import path updated to use new ChatTypes models.
Core import updates
front_end/panels/ai_chat/core/{AgentErrorHandler.ts,GraphHelpers.ts,State.ts,StateGraph.ts}, .../evaluation/remote/EvaluationAgent.ts, .../tools/FinalizeWithCritiqueTool.ts
Switched type/value imports from UI ChatView to models/ChatTypes; no logic changes.
Structured response core
front_end/panels/ai_chat/core/structured_response.ts
New parseStructuredResponse and getMessageStateKey utilities and StructuredResponse interface.
UI: AI chat panel & session components
front_end/panels/ai_chat/ui/AIChatPanel.ts, .../ui/LiveAgentSessionComponent.ts, .../ui/AgentSessionHeaderComponent.ts, .../ui/ToolCallComponent.ts, .../ui/ToolResultComponent.ts, .../ui/ToolDescriptionFormatter.ts
AIChatPanel: bound handlers, session upsert and test hooks; many new web components for live sessions, headers, tool call/result UIs and a shared ToolDescriptionFormatter utility.
UI: inputs & model selector
front_end/panels/ai_chat/ui/input/{ChatInput.ts,InputBar.ts}, front_end/panels/ai_chat/ui/model_selector/ModelSelector.ts
New ai-chat-input and ai-input-bar components, autosize/send behaviors, image preview, model selector component with searchable mode.
UI: markdown & message renderers
front_end/panels/ai_chat/ui/markdown/MarkdownRenderers.ts, .../ui/message/{MessageList.ts,MessageCombiner.ts,UserMessage.ts,ModelMessage.ts,ToolResultMessage.ts,StructuredResponseController.ts,StructuredResponseRender.ts}
New Markdown renderers (including TOC), ai-message-list with scroll behavior, message combiner to pair model tool-calls and results, renderers/controllers for user/model/tool/structured responses.
UI: version & oauth
front_end/panels/ai_chat/ui/version/VersionBanner.ts, .../ui/oauth/OAuthConnectPanel.ts
New version banner and OAuth connect panel web components.
Styles
front_end/panels/ai_chat/ui/chatView.css
Layout/input/z-index tweaks and input sizing adjustments.
Tests
front_end/panels/ai_chat/ui/__tests__/*, .../ui/input/__tests__/*, .../ui/message/__tests__/*, .../agent_framework/__tests__/*
Many new unit/integration tests covering sanitizer, AgentRunner flows, computeToolResultText, UI components (sessions, list, input, combiner, structured response), and AIChatPanel behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Panel as AIChatPanel
  participant Service as AgentService
  participant Runner as AgentRunner
  participant Bus as AgentRunnerEventBus
  participant LLM as LLM Provider
  participant Tools as Tool implementations
  participant UI as LiveAgentSessionComponent

  User->>Panel: send message
  Panel->>Service: submit/init (model, provider)
  Service->>Runner: run(messages, tools, {provider, getVisionCapability}, overrides)
  Runner->>Runner: getVisionCapability(model)
  Runner->>Sanitizer: sanitizeMessagesForModel(...)
  Runner->>Bus: emit(session_started)
  Bus-->>Service: agent-progress(session_started)
  Service->>UI: upsert session

  loop each tool
    Runner->>Bus: emit(tool_started)
    Bus-->>Service: agent-progress(tool_started)
    Service->>UI: add tool_call
    Runner->>Tools: execute(args, ctx{provider, model, getVisionCapability})
    Tools-->>Runner: tool_result
    Runner->>Bus: emit(tool_completed)
    Bus-->>Service: agent-progress(tool_completed)
    Service->>UI: update tool_result
  end

  Runner->>LLM: callWithMessages(sanitized)
  LLM-->>Runner: final answer / error
  Runner-->>Service: finalize session
  Service->>UI: finalize timeline
Loading
sequenceDiagram
  autonumber
  participant Runner as AgentRunner
  participant Cap as getVisionCapability
  participant San as MessageSanitizer
  participant LLM as LLM Provider

  Runner->>Cap: query(modelName)
  Cap-->>Runner: visionCapable (bool)
  Runner->>San: sanitizeMessagesForModel(messages, {visionCapable, placeholderForImageOnly:true})
  San-->>Runner: sanitizedMessages
  Runner->>LLM: call(sanitizedMessages, tools)
  LLM-->>Runner: response
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

  • Update agent framework structure #43 — Overlapping, code-level changes across the ai_chat agent framework and UI (AgentRunnerEventBus, MessageSanitizer, many ui components and provider/context plumbing).

Poem

I twitch my whiskers at the new event,
I hop through contexts that messages sent.
With sanitized crumbs and nested session tracks,
I pouch each tool-call and stitch the facts.
A rabbit's wiggle seals the patch — all set! 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/agent-updates

Comment @coderabbitai help to get the list of available commands and usage tips.

@tysonthomas9 tysonthomas9 requested a review from olesho September 6, 2025 15:26
Copy link

claude bot commented Sep 6, 2025

Claude finished @tysonthomas9's task —— View job


Comprehensive PR Code Review

This is a substantial refactoring of the agent framework with 71 files changed, adding 5924 lines and removing 2392 lines. Here's my detailed analysis:

Strong Points

  1. Excellent Test Coverage: Added 17+ comprehensive test files covering:

    • MessageSanitizer with edge cases and security scenarios
    • AgentRunner workflows and error handling
    • UI component interactions and nesting scenarios
    • Chat view functionality and agent sessions
  2. Good Architectural Improvements:

    • Clean separation of concerns with focused components
    • Event bus pattern for agent runner communication
    • Proper message sanitization for vision vs non-vision models
    • UI components broken into logical, reusable modules
  3. Security-Conscious Design:

    • MessageSanitizer.ts properly handles image data for different model capabilities
    • Deep cloning prevents mutation of original messages
    • Proper content type filtering

⚠️ Areas of Concern

1. Silent Error Handling

// front_end/panels/ai_chat/ui/LiveAgentSessionComponent.ts:49
try { this.#log.info('setSuppressInlineChildIds', { ids: Array.from(ids) }); } catch {}

Issue: Empty catch blocks suppress all errors, making debugging difficult. Consider logging errors or using specific error handling.

2. Potential Memory Leaks

// front_end/panels/ai_chat/ui/ChatView.ts:89
#liveSessionComponents = new Map<string, LiveAgentSessionComponent>();

Issue: Maps storing UI components may accumulate over time. No clear cleanup mechanism visible.
Recommendation: Implement proper cleanup in disconnectedCallback() or component lifecycle methods.

3. Performance Concerns

  • front_end/panels/ai_chat/ui/ChatView.ts: Multiple Maps and Sets created without clear size limits
  • Deep cloning in MessageSanitizer.ts uses JSON.parse(JSON.stringify()) which can be slow for large objects
  • Frequent re-rendering in UI components may impact performance with many agent sessions

4. Type Safety Issues

// front_end/panels/ai_chat/agent_framework/AgentRunner.ts:968
try { (toolResultData.agentSession as any).parentSessionId = currentSession.sessionId; } catch {}

Issue: Using as any bypasses type checking and combined with empty catch makes errors invisible.

🔍 Code Quality Observations

Positive:

  • Good use of TypeScript interfaces and type definitions
  • Consistent code formatting and structure
  • Proper separation between UI and business logic
  • Clear naming conventions

Areas for improvement:

  • Some complex functions could benefit from decomposition (e.g., AgentRunner.run())
  • Documentation could be enhanced for complex workflows
  • Consider using WeakMap for UI component caching to allow garbage collection

🚀 Performance Recommendations

  1. Message Sanitization: Consider caching sanitized messages or using a more efficient cloning method
  2. Component Management: Implement proper cleanup for cached components
  3. Event Handling: Review event listener cleanup in component lifecycle methods

🔒 Security Assessment

Good practices observed:

  • Input sanitization in MessageSanitizer.ts
  • Proper content filtering for different model capabilities
  • No obvious injection vulnerabilities in the changes

Minor concerns:

  • Empty catch blocks could mask security-relevant errors
  • Deep cloning with JSON might not handle all edge cases safely

📊 Test Coverage Assessment

Excellent coverage includes:

  • Unit tests for core functionality
  • Integration tests for UI components
  • Edge case testing for message handling
  • Agent session lifecycle testing

Suggestions:

  • Add performance benchmarks for large message sets
  • Consider adding memory leak tests for component cleanup

🎯 Overall Assessment

This is a well-architected refactoring that significantly improves code organization and testability. The addition of comprehensive tests is particularly commendable. The main concerns are around error handling patterns and potential memory management issues that should be addressed before merging.

Recommendation: Approve with minor revisions - address the silent error handling and add component cleanup mechanisms.


Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Comment on lines 446 to +460
};

// Run the agent
const ctx: any = _ctx || {};
const result = await AgentRunner.run(
internalMessages,
args,
runnerConfig,
runnerHooks,
this // Pass the current agent instance as executingAgent
this, // executingAgent
undefined,
{
sessionId: ctx.overrideSessionId,
parentSessionId: ctx.overrideParentSessionId,
traceId: ctx.overrideTraceId,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Nested agents drop parent LLM provider overrides

When a configurable agent is called as a tool, AgentRunner passes the parent session’s provider and vision-capability function through the _ctx argument. The execute implementation ignores these values and always derives the provider via AIChatPanel.getProviderForModel(modelName). In flows where the parent agent runs with a non-default provider (e.g. a custom or remote provider selected at runtime), the child agent will silently fall back to the default provider and may fail due to missing credentials or inconsistent capabilities. Consider honoring _ctx.provider and _ctx.getVisionCapability when present so nested agent executions continue to use the same LLM configuration as the parent.

Useful? React with 👍 / 👎.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 36

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (26)
front_end/panels/ai_assistance/components/ChatView.ts (1)

1446-1447: Fix event and target typing for textarea input.

This is a textarea, not an input, and the handler is an input event, not a keyboard event. Avoids incorrect casts and TS friction.

-        @input=${(event: KeyboardEvent) => onTextInputChange((event.target as HTMLInputElement).value)}
+        @input=${(event: Event) => onTextInputChange((event.target as HTMLTextAreaElement).value)}
front_end/panels/ai_chat/core/GraphHelpers.ts (2)

44-47: Use explicit default agent type instead of empty string.

Passing '' relies on downstream fallback; use 'default' for determinism.

 export function getAgentToolsFromState(state: AgentState): ReturnType<typeof getTools> {
   // Use the helper from BaseOrchestratorAgent to get the pre-filtered list
-  return BaseOrchestratorAgent.getAgentTools(state.selectedAgentType ?? ''); // Pass agentType or empty string
+  return BaseOrchestratorAgent.getAgentTools(state.selectedAgentType ?? 'default');
 }

18-28: Migrate remaining synchronous createSystemPrompt calls to async version
– In front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts replace the call at line 104 (this.createSystemPrompt()) with an awaited this.createSystemPromptAsync(state) (and mark the enclosing method as async), then deprecate or remove the private sync createSystemPrompt implementation at line 158.

front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (3)

402-416: Fallback to action_agent uses wrong input schema.

action_agent requires { objective, reasoning } per its schema; passing { task, reasoning } will fail validation.

Apply:

-                { 
-                  task: `Navigate to ${params.url}`,
-                  reasoning: 'Navigation required for evaluation'
-                },
+                {
+                  objective: `Navigate to ${params.url}`,
+                  reasoning: 'Navigation required for evaluation'
+                },

438-446: Chat model override is ignored.

You merge params.model into mergedInput but executeChatEvaluation always takes model from localStorage/default, discarding overrides.

Apply:

-        // Use the current model from localStorage (no override)
-        let modelName = localStorage.getItem('ai_chat_model_selection');
-        if (!modelName) {
-          // Default model
-          modelName = 'gpt-4o';
-        }
+        // Prefer explicit input override, then local selection, then default
+        let modelName =
+          (input?.main_model as string | undefined) ||
+          localStorage.getItem('ai_chat_model_selection') ||
+          'gpt-4o';

Also applies to: 705-719


281-290: Disconnect on failed secret verification.

After sending a negative verification, reject and disconnect to avoid dangling connections.

Apply:

       if (!verified) {
         logger.error('Secret key verification failed - keys do not match');
         // Reject auth promise immediately since we know auth will fail
         if (this.authReject) {
           this.authReject(new Error('Secret key verification failed - keys do not match'));
           this.authResolve = null;
           this.authReject = null;
         }
+        this.disconnect();
       }
front_end/panels/ai_chat/core/StateGraph.ts (1)

72-99: Tracing calls should not be able to break graph execution.

createObservation calls are awaited without try/catch; a tracing backend hiccup will abort the graph.

Apply:

-      let spanId: string | undefined;
       const spanStartTime = new Date();
 
       if (tracingContext?.traceId) {
-        spanId = `span-${currentNodeName}-${Date.now()}-${Math.random().toString(36).substring(2, 9)}`;
-        
-        // Create a start event for the node
-        await this.tracingProvider.createObservation({
+        // Create a start event for the node
+        try {
+          await this.tracingProvider.createObservation({
             id: `event-start-${currentNodeName}-${Date.now()}`,
             name: `Start Node: ${currentNodeName}`,
             type: 'event',
             startTime: spanStartTime,
             parentObservationId: tracingContext.parentObservationId,
             input: {
               nodeType: currentNodeName,
               graphName: this.name,
               step,
               messagesCount: currentState.messages?.length || 0
             },
             metadata: {
               nodeType: currentNodeName,
               graphName: this.name,
               step,
               phase: 'start'
             }
-        }, tracingContext.traceId);
+          }, tracingContext.traceId);
+        } catch (e) {
+          logger.warn('Tracing start event failed:', e);
+        }
       }
@@
-        if (spanId && tracingContext?.traceId) {
-          await this.tracingProvider.createObservation({
+        if (tracingContext?.traceId) {
+          try {
+            await this.tracingProvider.createObservation({
               id: `event-complete-${currentNodeName}-${Date.now()}`,
               name: `Complete Node: ${currentNodeName}`,
               type: 'event',
               startTime: new Date(),
               parentObservationId: tracingContext.parentObservationId,
               output: { 
                 success: true,
                 duration: Date.now() - spanStartTime.getTime(),
                 messagesAfter: currentState.messages?.length || 0
               },
               metadata: {
                 nodeType: currentNodeName,
                 graphName: this.name,
                 step,
                 phase: 'complete'
               }
-          }, tracingContext.traceId);
+            }, tracingContext.traceId);
+          } catch (e) {
+            logger.warn('Tracing completion event failed:', e);
+          }
         }
@@
-        if (spanId && tracingContext?.traceId) {
-          await this.tracingProvider.createObservation({
+        if (tracingContext?.traceId) {
+          try {
+            await this.tracingProvider.createObservation({
               id: `event-error-${currentNodeName}-${Date.now()}`,
               name: `Error Node: ${currentNodeName}`,
               type: 'event',
               startTime: new Date(),
               parentObservationId: tracingContext.parentObservationId,
               error: error instanceof Error ? error.message : String(error),
               metadata: {
                 nodeType: currentNodeName,
                 graphName: this.name,
                 step,
                 phase: 'error'
               }
-          }, tracingContext.traceId);
+            }, tracingContext.traceId);
+          } catch (e) {
+            logger.warn('Tracing error event failed:', e);
+          }
         }

Also applies to: 105-124, 132-147

front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (2)

620-641: Action Verification Agent references unregistered tools.

inspect_element and get_console_logs are not registered/imported; tool calls will fail at runtime.

Either register these tools or remove them from the agent’s tool list and prompt. Minimal fix:

-## Tools to Use
-- get_page_content: Examine the updated page structure
-- search_content: Look for specific text indicating success/failure
-- inspect_element: Check properties of specific elements
-- get_console_logs: Check for errors or success messages in the console
-- schema_based_extractor: Extract structured data to verify expected outcomes
-- take_screenshot: Capture the current page state
+## Tools to Use
+- get_page_content: Examine the updated page structure
+- search_content: Look for specific text indicating success/failure
+- schema_based_extractor: Extract structured data to verify expected outcomes
+- take_screenshot: Capture the current page state
@@
-    tools: [
-      'search_content',
-      'inspect_element',
-      'get_console_logs',
-      'schema_based_extractor',
-      'take_screenshot'
-    ],
+    tools: ['search_content', 'schema_based_extractor', 'take_screenshot'],

If you intended to support those tools, I can add the implementations and registrations instead.


1421-1423: E‑commerce agent prompt vs tools mismatch.

System prompt instructs use of schema_based_extractor and search_content, but they’re not in tools[]. Add them.

Apply:

-    tools: [
-      'navigate_url',
-      'get_page_content',
-    ],
+    tools: [
+      'navigate_url',
+      'get_page_content',
+      'schema_based_extractor',
+      'search_content',
+    ],
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts (2)

55-76: Fix tool schema: required keys mismatch and missing extractionInstruction.

Schema marks schema and markdownResponse as required, but the args interface makes them optional. Also, extractionInstruction is used but not declared in properties. This will cause validation failures or dropped fields.

Apply:

   schema = {
     type: 'object',
     properties: {
       url: {
         type: 'string',
         description: 'URL to navigate to'
       },
       schema: {
         type: 'object',
         description: 'JSON Schema definition of the data to extract'
       },
       markdownResponse: {
         type: 'boolean',
         description: 'If true, converts the page content to Markdown'
       },
       reasoning: {
         type: 'string',
         description: 'Reasoning for the action displayed to the user'
       },
+      extractionInstruction: {
+        type: 'string',
+        description: 'Optional instruction to guide extraction/markdown conversion'
+      },
     },
-    required: ['url', 'reasoning', 'schema', 'markdownResponse']
+    required: ['url', 'reasoning']
   };

81-93: Validate ctx when sub-tools need LLM context; fail fast with a clear message.

Both SchemaBasedExtractorTool and HTMLToMarkdownTool now consume ctx; without provider/model this can throw at runtime.

Minimal guard:

   async execute(args: CombinedExtractionArgs, ctx?: LLMContext): Promise<CombinedExtractionResult | ErrorResult> {
     logger.info('Executing with args', { args });
     const { url, schema, markdownResponse, reasoning, extractionInstruction } = args;
     const agentService = AgentService.getInstance();
     const apiKey = agentService.getApiKey();

     if (!apiKey) {
       return {
         success: false,
         url,
         error: 'API key not configured'
       };
     }
+
+    // If either operation may rely on LLM, ensure ctx is present
+    if ((schema || markdownResponse) && (!ctx?.provider || !ctx?.model)) {
+      return {
+        success: false,
+        url,
+        error: 'LLM context missing provider/model'
+      };
+    }

Also applies to: 131-141, 149-157

front_end/panels/ai_chat/tools/CritiqueTool.ts (2)

51-51: Fix user-facing typo

“finalresponse” → “final response”.

-  description = 'Evaluates if finalresponse satisfies the user\'s requirements and provides feedback if needed.';
+  description = 'Evaluates if final response satisfies the user\'s requirements and provides feedback if needed.';

82-89: Align input validation with schema (also require reasoning)

Schema marks reasoning as required; validate it too and clarify error.

-    if (!userInput || !finalResponse) {
+    if (!userInput || !finalResponse || !reasoning) {
       return {
         satisfiesCriteria: false,
         success: false,
-        error: 'Both user input and final response must be provided.'
+        error: 'userInput, finalResponse, and reasoning must be provided.'
       };
     }
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (1)

77-91: Fail fast if LLM context is missing

Multiple inner calls throw later; validate up-front for clearer errors.

 async execute(args: SchemaExtractionArgs, ctx?: LLMContext): Promise<SchemaExtractionResult> {
   logger.debug('Executing with args', args);
 
   const { schema, instruction, reasoning } = args;
   const agentService = AgentService.getInstance();
   const apiKey = agentService.getApiKey();
 
   if (!apiKey) {
     return {
       success: false,
       data: null,
       error: 'API key not configured'
     };
   }
+  if (!ctx?.provider || !(ctx.nanoModel || ctx.model)) {
+    return {
+      success: false,
+      data: null,
+      error: 'Missing LLM context (provider/model)'
+    };
+  }
front_end/panels/ai_chat/tools/SequentialThinkingTool.ts (1)

225-229: Possible invalid image payload across providers (data URL vs http URL).

If screenshot is base64 without data: prefix, many providers will reject image_url.url. Normalize before send.

-      images: [{
-        type: 'image_url',
-        data: visualContext.screenshot
-      }]
+      images: [{
+        type: 'image_url',
+        data: visualContext.screenshot
+      }]

And in getGroundedAnalysis:

-        const validImages = prompt.images.filter(img => !!img?.data && img.data !== 'no-screenshot-available');
+        const toUrl = (s: string) => s.startsWith('http') || s.startsWith('data:')
+          ? s
+          : `data:image/png;base64,${s}`;
+        const validImages = prompt.images.filter(img => !!img?.data && img.data !== 'no-screenshot-available');
...
-            ...validImages.map(img => ({
+            ...validImages.map(img => ({
               type: 'image_url' as const,
-              image_url: { url: img.data }
+              image_url: { url: toUrl(img.data) }
             }))

Also applies to: 255-265

front_end/panels/ai_chat/ui/AIChatPanel.test.ts (2)

204-221: Assertion uses Map.get(...) instead of window.localStorage.getItem(...).

mockLocalStorage.get(...) returns undefined, not null. The test will fail intermittently.

-      assert.strictEqual(mockLocalStorage.get('ai_chat_mini_model'), null);
+      assert.isNull(window.localStorage.getItem('ai_chat_mini_model'));

350-373: Cannot assign to private fields (#agentService, #canSendMessages) from tests.

(panel as any)['#agentService'] and ['#canSendMessages'] won’t patch real private fields; the test won’t mock sendMessage as intended.

Two fixes; pick one:

A) Add a testing hook in AIChatPanel (preferred).

// In AIChatPanel.ts
+ /** TEST-ONLY: inject a mock AgentService */
+ static setAgentServiceForTesting(svc: Pick<AgentService, 'sendMessage'>) {
+   AIChatPanel.instance()['#agentService'] = svc as any;
+ }

And in the test:

-      (panel as any)['#agentService'] = mockAgentService;
-      (panel as any)['#canSendMessages'] = true;
+      AIChatPanel.setAgentServiceForTesting(mockAgentService as any);

B) Monkey-patch AgentService.getInstance in the test.

-      const mockAgentService = { sendMessage: async () => Promise.resolve(), };
-      (panel as any)['#agentService'] = mockAgentService;
-      (panel as any)['#canSendMessages'] = true;
+      const mockAgentService = { sendMessage: async () => Promise.resolve() } as any;
+      const originalGetInstance = AgentService.getInstance;
+      // @ts-ignore
+      AgentService.getInstance = () => mockAgentService;
+      try {
+        await panel.sendMessage('test message');
+      } finally {
+        AgentService.getInstance = originalGetInstance;
+      }
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)

167-191: String interpolation bug in logger.

Template string is in single quotes; logs ${name} literally.

-        logger.info('Registered and instantiated tool: ${name}');
+        logger.info(`Registered and instantiated tool: ${name}`);

386-413: Expose requiresApiKey() on AgentService and use it for gating
Add a public requiresApiKey() wrapper around the private #doesCurrentConfigRequireApiKey() in core/AgentService.ts and update ConfigurableAgentTool.ts to only hard‐fail when that returns true:

 // ConfigurableAgentTool.ts
   const apiKey = agentService.getApiKey();
-  if (!apiKey) {
+  const requiresKey = agentService.requiresApiKey();
+  if (requiresKey && !apiKey) {
     const errorResult = this.createErrorResult(`API key not configured for ${this.name}`, [], 'error');
     …
     return { …errorResult, agentSession: errorSession };
   }
front_end/panels/ai_chat/core/ConfigurableGraph.ts (1)

32-43: Add provider/modelName to base graph configurations
front_end/panels/ai_chat/core/GraphConfigs.ts (lines 15–20) defines agent/toolExecutor nodes without provider or modelName, causing createAgentGraphFromConfig to fail.

front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)

271-275: Bug: LLM JSON not parsed (breaks URL resolution and retry paths)

llmResponse.text is returned as a string; downstream resolvers expect objects. Parse before returning.

-        const result = llmResponse.text;
-        
-        logger.debug(`JSON extraction successful on attempt ${attempt}`);
-        return result;
+        const raw = llmResponse.text;
+        const parsed = typeof raw === 'string' ? JSON.parse(raw) : raw;
+        logger.debug(`JSON extraction successful on attempt ${attempt}`);
+        return parsed;
-      const result = llmResponse.text;
-      
-      return result;
+      const raw = llmResponse.text;
+      const parsed = typeof raw === 'string' ? JSON.parse(raw) : raw;
+      return parsed;

Also applies to: 414-416

front_end/panels/ai_chat/ui/AIChatPanel.ts (2)

49-72: Stop logging secrets; gate the localStorage monkey-patch behind an explicit debug flag

The storage monitor currently logs value previews/lengths for keys that are likely to contain API keys/tokens and is always active. This risks credential leakage to logs and alters global behavior in production.

Apply masking and an opt-in flag:

   private setupStorageMonitoring(): void {
     // Monitor setItem operations
-    localStorage.setItem = (key: string, value: string) => {
-      if (key.includes('openrouter') || key.includes('ai_chat')) {
+    localStorage.setItem = (key: string, value: string) => {
+      const shouldLog = (key.includes('openrouter') || key.includes('ai_chat')) &&
+        localStorage.getItem('ai_chat_debug_storage_monitor') === 'true';
+      if (shouldLog) {
+        const isSensitive = /api_key|secret|token|auth/i.test(key);
         logger.debug(`=== LOCALSTORAGE SET ===`);
         logger.debug(`Key: ${key}`);
-        logger.debug(`Value exists: ${!!value}`);
-        logger.debug(`Value length: ${value?.length || 0}`);
-        logger.debug(`Value preview: ${value?.substring(0, 50) + (value?.length > 50 ? '...' : '') || 'null'}`);
+        logger.debug(`Value exists: ${!!value}`);
+        if (!isSensitive) {
+          logger.debug(`Value length: ${value?.length || 0}`);
+          logger.debug(`Value preview: ${value?.substring(0, 50) + (value?.length > 50 ? '...' : '') || 'null'}`);
+        } else {
+          logger.debug(`Value: <redacted>`);
+        }
         logger.debug(`Timestamp: ${new Date().toISOString()}`);
       }
       return this.originalSetItem(key, value);
     };
     
     // Monitor removeItem operations
-    localStorage.removeItem = (key: string) => {
-      if (key.includes('openrouter') || key.includes('ai_chat')) {
+    localStorage.removeItem = (key: string) => {
+      const shouldLog = (key.includes('openrouter') || key.includes('ai_chat')) &&
+        localStorage.getItem('ai_chat_debug_storage_monitor') === 'true';
+      if (shouldLog) {
         logger.debug(`=== LOCALSTORAGE REMOVE ===`);
         logger.debug(`Key: ${key}`);
         logger.debug(`Timestamp: ${new Date().toISOString()}`);
       }
       return this.originalRemoveItem(key);
     };
   }

Additionally consider only constructing StorageMonitor when the same flag is set to 'true'.


644-651: Bug: openrouter custom models don’t update MODEL_OPTIONS when openrouter is selected

The current-provider branch omits openrouter.

-    if ((currentProvider === 'openai' && modelType === 'openai') || 
-        (currentProvider === 'litellm' && modelType === 'litellm') ||
-        (currentProvider === 'groq' && modelType === 'groq')) {
+    if ((currentProvider === 'openai' && modelType === 'openai') || 
+        (currentProvider === 'litellm' && modelType === 'litellm') ||
+        (currentProvider === 'groq' && modelType === 'groq') ||
+        (currentProvider === 'openrouter' && modelType === 'openrouter')) {
       MODEL_OPTIONS = [...MODEL_OPTIONS, newOption];
       localStorage.setItem('ai_chat_model_options', JSON.stringify(MODEL_OPTIONS));
     }
front_end/panels/ai_chat/core/AgentService.ts (1)

466-473: Type mismatch: adding non-declared error field to ModelChatMessage.

Either extend the type or intersect when constructing the error message to satisfy TS.

-      const errorMessage: ModelChatMessage = {
+      const errorMessage: ModelChatMessage & { error: string } = {
         entity: ChatMessageEntity.MODEL,
         action: 'final',
         answer: error instanceof Error ? error.message : String(error),
         isFinalAnswer: true,
         error: error instanceof Error ? error.message : String(error),
       };

Alternatively, add error?: string to ModelChatMessage in ChatTypes.ts.

front_end/panels/ai_chat/agent_framework/AgentRunner.ts (2)

161-184: Deep-sanitize tool results and redact secrets before LLM/log storage

Current sanitization is shallow and may leak large base64 blobs or secrets (apiKey, Authorization, cookies) via nested objects.

Apply this diff to replace the method with a recursive, size-aware redactor:

   private static sanitizeToolResultForText(toolResultData: any): any {
-    if (typeof toolResultData !== 'object' || toolResultData === null) {
-      return toolResultData;
-    }
-
-    // Create a shallow copy
-    const sanitized = { ...toolResultData };
-
-    // Remove fields that shouldn't be sent to LLM
-    const fieldsToRemove = [
-      'imageData',    // Prevents token waste from base64 strings
-      'success',      // LLM should infer success from error presence
-      'dataUrl',      // Legacy image field if any
-      'agentSession', // Avoid sending session data to LLM
-    ];
-
-    fieldsToRemove.forEach(field => {
-      if (sanitized.hasOwnProperty(field)) {
-        delete sanitized[field];
-      }
-    });
-
-    return sanitized;
+    if (toolResultData === null || typeof toolResultData !== 'object') {
+      return toolResultData;
+    }
+
+    const redactedKeys = new Set([
+      'imageData', 'dataUrl', 'screenshot', 'blob', 'file', 'binary', 'base64',
+      'agentSession', 'headers', 'cookies', 'setCookie',
+      'apiKey', 'apikey', 'token', 'accessToken', 'authorization', 'authorizationHeader',
+    ]);
+
+    const visited = new WeakSet<object>();
+    const MAX_DEPTH = 4;
+    const MAX_STRING = 10_000;
+
+    const redact = (val: any, depth: number): any => {
+      if (val === null || typeof val !== 'object') {
+        if (typeof val === 'string' && val.length > MAX_STRING) {
+          return `[omitted ${val.length} chars]`;
+        }
+        return val;
+      }
+      if (visited.has(val) || depth > MAX_DEPTH) {
+        return '[omitted]';
+      }
+      visited.add(val);
+
+      if (Array.isArray(val)) {
+        return val.map(v => redact(v, depth + 1));
+      }
+
+      const out: Record<string, any> = {};
+      for (const [k, v] of Object.entries(val)) {
+        if (redactedKeys.has(k)) {
+          continue; // drop entirely
+        }
+        out[k] = redact(v, depth + 1);
+      }
+      return out;
+    };
+
+    return redact(toolResultData, 0);
   }

1072-1081: Store sanitized results in messages to prevent base64 bloat/PII retention

Use the sanitizer for both the LLM-facing ToolResultMessage and the session timeline message.

Apply this diff:

           const toolResultMessage: ToolResultMessage = {
             entity: ChatMessageEntity.TOOL_RESULT,
             toolName,
             resultText: toolResultText,
             isError: toolIsError,
             toolCallId, // Link back to the tool call for OpenAI format
             ...(toolIsError && { error: toolResultText }), // Include raw error message if error occurred
-            ...(toolResultData && { resultData: toolResultData }), // Include structured result data
+            ...(toolResultData && { resultData: AgentRunner.sanitizeToolResultForText(toolResultData) }), // Include sanitized data
             ...(imageData && { imageData: imageData }) // Include image data for multimodal LLM responses
           };
           addSessionMessage({
             type: 'tool_result',
             content: {
               type: 'tool_result',
               toolCallId,
               toolName,
               success: !toolIsError,
-              result: toolResultData,
+              result: AgentRunner.sanitizeToolResultForText(toolResultData),
               error: toolIsError ? toolResultText : undefined
             }
           });

Also applies to: 1093-1103

Comment on lines +606 to 611
"front_end/panels/ai_chat/ui/LiveAgentSessionComponent.js",
"front_end/panels/ai_chat/ui/ToolCallComponent.js",
"front_end/panels/ai_chat/ui/ToolResultComponent.js",
"front_end/panels/ai_chat/ui/AgentSessionHeaderComponent.js",
"front_end/panels/ai_chat/ui/ToolDescriptionFormatter.js",
"front_end/panels/ai_chat/ui/chatView.css.js",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

New AI Chat UI files added to GRD manifest — ensure files exist.

Good additions. Please verify the files exist at these paths (case and subdir correctness), otherwise grd gen will break.


🏁 Script executed:

#!/bin/bash
paths=(
front_end/panels/ai_chat/ui/LiveAgentSessionComponent.js
front_end/panels/ai_chat/ui/ToolCallComponent.js
front_end/panels/ai_chat/ui/ToolResultComponent.js
front_end/panels/ai_chat/ui/AgentSessionHeaderComponent.js
front_end/panels/ai_chat/ui/ToolDescriptionFormatter.js
)
for p in "${paths[@]}"; do [[ -f "$p" ]] || echo "MISSING: $p"; done

Length of output: 1412


Correct missing AI Chat UI component entries in config/gni/devtools_grd_files.gni.

The manifest lists these files, but none exist at the given paths—update to the actual filenames (e.g. .tsx extensions) or remove the invalid entries:

  • front_end/panels/ai_chat/ui/LiveAgentSessionComponent.js
  • front_end/panels/ai_chat/ui/ToolCallComponent.js
  • front_end/panels/ai_chat/ui/ToolResultComponent.js
  • front_end/panels/ai_chat/ui/AgentSessionHeaderComponent.js
  • front_end/panels/ai_chat/ui/ToolDescriptionFormatter.js
🤖 Prompt for AI Agents
In config/gni/devtools_grd_files.gni around lines 606 to 611, the manifest lists
several AI Chat UI files that do not exist at the given .js paths; update the
entries to point to the actual filenames (e.g. replace .js with the correct
.tsx/.jsx names) or remove the invalid lines. Verify the real file locations and
extensions in front_end/panels/ai_chat/ui, update each listed entry
(LiveAgentSessionComponent, ToolCallComponent, ToolResultComponent,
AgentSessionHeaderComponent, ToolDescriptionFormatter, chatView.css.js) to the
correct filenames or drop them from the manifest, then run a build/check to
ensure no missing-file errors remain.

Comment on lines +662 to 678
"front_end/panels/ai_chat/core/structured_response.js",
"front_end/panels/ai_chat/models/ChatTypes.js",
"front_end/panels/ai_chat/ui/input/ChatInput.js",
"front_end/panels/ai_chat/ui/input/InputBar.js",
"front_end/panels/ai_chat/ui/markdown/MarkdownRenderers.js",
"front_end/panels/ai_chat/ui/message/MessageList.js",
"front_end/panels/ai_chat/ui/message/ModelMessage.js",
"front_end/panels/ai_chat/ui/message/MessageCombiner.js",
"front_end/panels/ai_chat/ui/message/StructuredResponseRender.js",
"front_end/panels/ai_chat/ui/message/StructuredResponseController.js",
"front_end/panels/ai_chat/ui/message/GlobalActionsRow.js",
"front_end/panels/ai_chat/ui/message/ToolResultMessage.js",
"front_end/panels/ai_chat/ui/message/UserMessage.js",
"front_end/panels/ai_chat/ui/model_selector/ModelSelector.js",
"front_end/panels/ai_chat/ui/oauth/OAuthConnectPanel.js",
"front_end/panels/ai_chat/ui/version/VersionBanner.js",
"front_end/panels/ai_chat/common/WebSocketRPCClient.js",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Bulk add of AI Chat core/models/ui files — sanity-check presence and typos.

Large manifest updates are brittle. Quick presence check avoids GRD churn.


🏁 Script executed:

#!/bin/bash
paths=(
front_end/panels/ai_chat/core/structured_response.js
front_end/panels/ai_chat/models/ChatTypes.js
front_end/panels/ai_chat/ui/input/ChatInput.js
front_end/panels/ai_chat/ui/input/InputBar.js
front_end/panels/ai_chat/ui/markdown/MarkdownRenderers.js
front_end/panels/ai_chat/ui/message/MessageList.js
front_end/panels/ai_chat/ui/message/ModelMessage.js
front_end/panels/ai_chat/ui/message/MessageCombiner.js
front_end/panels/ai_chat/ui/message/StructuredResponseRender.js
front_end/panels/ai_chat/ui/message/StructuredResponseController.js
front_end/panels/ai_chat/ui/message/GlobalActionsRow.js
front_end/panels/ai_chat/ui/message/ToolResultMessage.js
front_end/panels/ai_chat/ui/message/UserMessage.js
front_end/panels/ai_chat/ui/model_selector/ModelSelector.js
front_end/panels/ai_chat/ui/oauth/OAuthConnectPanel.js
front_end/panels/ai_chat/ui/version/VersionBanner.js
)
missing=0; for p in "${paths[@]}"; do [[ -f "$p" ]] || { echo "MISSING: $p"; missing=1; }; done; exit $missing

Length of output: 4714


Manifest references nonexistent AI Chat files
All 16 entries added in config/gni/devtools_grd_files.gni (lines 662–678) point to files that aren’t present in the repository. Correct the paths or add the missing source files before landing this change.

🤖 Prompt for AI Agents
In config/gni/devtools_grd_files.gni around lines 662–678 the 16 added entries
reference AI Chat source files that do not exist in the repo; either add the
missing files to the repository at the paths listed, or correct the paths to the
actual file locations (or remove the entries if those features are not ready).
Verify each filename and path against the source tree, add the missing JS
modules with the expected exports if needed, update the GN manifest to the
correct relative paths, and run the build/gn check to ensure no unresolved file
references remain before landing.

"front_end/panels/ai_chat/ai_chat.js",
"front_end/panels/ai_chat/ai_chat_impl.js",
"front_end/panels/ai_chat/agent_framework/AgentRunner.js",
"front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.js",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

AgentRunnerEventBus added — good; verify usage and existence.

This aligns with the agent runtime changes. Confirm the file exists and at least one import references it.


🏁 Script executed:

#!/bin/bash
# File existence
test -f front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.js || echo "MISSING: AgentRunnerEventBus.js"
# Referenced by AgentRunner or others
rg -n 'AgentRunnerEventBus' front_end/panels/ai_chat

Length of output: 1716


🏁 Script executed:

#!/bin/bash
# Verify TypeScript source exists
test -f front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts || echo "MISSING: AgentRunnerEventBus.ts"

Length of output: 73


Fix missing AgentRunnerEventBus.js in devtools_grd_files.gni

config/gni/devtools_grd_files.gni lists front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.js but no .js exists in source (only AgentRunnerEventBus.ts), so update this entry to point at the generated .js artifact or switch to the .ts path to match the codebase.

🤖 Prompt for AI Agents
In config/gni/devtools_grd_files.gni around line 691, the list contains
"front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.js" but only
AgentRunnerEventBus.ts exists in source; update this entry to point to the
TypeScript source
"front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts" (or
alternatively replace it with the correct generated JS artifact path used by
your build output), and verify the GN rules include .ts files or the generated
.js location so the file is present in the built GRD.

Comment on lines +32 to +33
"panels/ai_chat:ai_chat_release_js_metadata",
"panels/ai_chat:ai_chat_release_css_metadata",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Additions look correct; verify targets exist across build configs.

The new ai_chat release metadata deps make sense for flowing GRD metadata. Please confirm these GN targets are defined (and not gated behind a condition that would make them unavailable in some configs), otherwise the root group will fail to resolve.

Run to confirm target definitions and single inclusion:


🏁 Script executed:

#!/bin/bash
# 1) Targets exist somewhere under front_end/**/BUILD.gn
rg -n 'ai_chat_release_(js|css)_metadata' front_end/**/BUILD.gn -C2

# 2) Only referenced once in front_end/BUILD.gn
rg -n 'panels/ai_chat:ai_chat_release_(js|css)_metadata' front_end/BUILD.gn

Length of output: 352


Define missing GN targets or remove dependencies
The targets panels/ai_chat:ai_chat_release_js_metadata and panels/ai_chat:ai_chat_release_css_metadata aren’t defined in any front_end/**/BUILD.gn (no matches found via ripgrep). The root GN group will fail to resolve these deps—please add or correct these target definitions.

🤖 Prompt for AI Agents
In front_end/BUILD.gn around lines 32-33, the GN deps reference two targets that
don't exist: panels/ai_chat:ai_chat_release_js_metadata and
panels/ai_chat:ai_chat_release_css_metadata; add definitions for these targets
(or remove the dependencies) in panels/ai_chat/BUILD.gn so the root group can
resolve them. To fix, open panels/ai_chat/BUILD.gn and add two metadata/group
(or action) targets named ai_chat_release_js_metadata and
ai_chat_release_css_metadata that expose the JS and CSS release metadata files
or depend on the actual targets that produce those artifacts, ensuring names
exactly match the deps, or alternatively remove the two lines from the root
BUILD.gn if those artifacts are not required.

Comment on lines +5 to +11
import { AgentRunner } from '../AgentRunner.js';
import type { AgentRunnerConfig, AgentRunnerHooks } from '../AgentRunner.js';
import { ChatMessageEntity, type ChatMessage } from '../../models/ChatTypes.js';
import type { Tool } from '../../tools/Tools.js';
import { AIChatPanel } from '../../ui/AIChatPanel.js';
import { LLMClient } from '../../LLM/LLMClient.js';

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing assert import breaks tests

Tests use chai’s assert API (assert.isOk, assert.isFalse) but don’t import it.

Apply:

 import { AgentRunner } from '../AgentRunner.js';
 import type { AgentRunnerConfig, AgentRunnerHooks } from '../AgentRunner.js';
 import { ChatMessageEntity, type ChatMessage } from '../../models/ChatTypes.js';
 import type { Tool } from '../../tools/Tools.js';
 import { AIChatPanel } from '../../ui/AIChatPanel.js';
 import { LLMClient } from '../../LLM/LLMClient.js';
+import { assert } from 'chai';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { AgentRunner } from '../AgentRunner.js';
import type { AgentRunnerConfig, AgentRunnerHooks } from '../AgentRunner.js';
import { ChatMessageEntity, type ChatMessage } from '../../models/ChatTypes.js';
import type { Tool } from '../../tools/Tools.js';
import { AIChatPanel } from '../../ui/AIChatPanel.js';
import { LLMClient } from '../../LLM/LLMClient.js';
import { AgentRunner } from '../AgentRunner.js';
import type { AgentRunnerConfig, AgentRunnerHooks } from '../AgentRunner.js';
import { ChatMessageEntity, type ChatMessage } from '../../models/ChatTypes.js';
import type { Tool } from '../../tools/Tools.js';
import { AIChatPanel } from '../../ui/AIChatPanel.js';
import { LLMClient } from '../../LLM/LLMClient.js';
import { assert } from 'chai';
🤖 Prompt for AI Agents
In
front_end/panels/ai_chat/agent_framework/__tests__/AgentRunner.run.flows.test.ts
around lines 5 to 11, the test file calls chai's assert methods (e.g.,
assert.isOk, assert.isFalse) but never imports assert; add an import for assert
from chai (e.g., import { assert } from 'chai';) near the top with the other
imports so the assert symbols are defined and the tests run.

Comment on lines +7 to +9
const {html, Decorators} = Lit;
const {customElement} = Decorators as any;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove as any and keep decorators typed

Avoid any to satisfy lint and keep types. Replace destructure to use the typed decorators.

-const {html, Decorators} = Lit;
-const {customElement} = Decorators as any;
+const {html, Decorators} = Lit;
+const {customElement} = Decorators;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const {html, Decorators} = Lit;
const {customElement} = Decorators as any;
// front_end/panels/ai_chat/ui/version/VersionBanner.ts
// before (lines 7–9)
// const {html, Decorators} = Lit;
// const {customElement} = Decorators as any;
// after
const {html, Decorators} = Lit;
const {customElement} = Decorators;
🧰 Tools
🪛 ESLint

[error] 8-8: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/version/VersionBanner.ts around lines 7-9, remove
the "as any" cast on Decorators and destructure using the actual typed property
on Lit; e.g. keep html from Lit and obtain customElement directly from
Lit.Decorators so the decorators remain properly typed (replace the current
destructure with separate accesses like getting html from Lit and customElement
from Lit.Decorators) to eliminate the any cast and satisfy the linter.

const {html, Decorators} = Lit;
const {customElement} = Decorators as any;

export interface VersionInfo { latestVersion: string; releaseUrl: string; isUpdateAvailable: boolean; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix interface member delimiter style (ESLint: member-delimiter-style)

Your config rejects semicolons in single-line interfaces. Convert to multiline with comma delimiters (also for HTMLElementTagNameMap).

-export interface VersionInfo { latestVersion: string; releaseUrl: string; isUpdateAvailable: boolean; }
+export interface VersionInfo {
+  latestVersion: string,
+  releaseUrl: string,
+  isUpdateAvailable: boolean,
+}
-declare global {
-  interface HTMLElementTagNameMap { 'ai-version-banner': VersionBanner; }
-}
+declare global {
+  interface HTMLElementTagNameMap {
+    'ai-version-banner': VersionBanner,
+  }
+}

Also applies to: 48-50

🧰 Tools
🪛 ESLint

[error] 10-10: Unexpected separator (;).

(@stylistic/member-delimiter-style)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/version/VersionBanner.ts around line 10 (and also
for the interface at lines 48-50), the single-line interface uses semicolon
delimiters which violates the member-delimiter-style rule; change each
single-line interface to a multiline form where each property is on its own line
and terminated with a comma (not a semicolon), and apply the same multiline
comma-delimited style to the HTMLElementTagNameMap declaration mentioned at
lines 48-50.

export interface VersionInfo { latestVersion: string; releaseUrl: string; isUpdateAvailable: boolean; }

@customElement('ai-version-banner')
export class VersionBanner extends HTMLElement {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Move component under a components/ folder to satisfy lint

Rule requires custom elements to live in ui/components/ or a panel components/ subfolder. Suggest: front_end/panels/ai_chat/ui/components/VersionBanner.ts and update imports/BUILD.gn.

🧰 Tools
🪛 ESLint

[error] 13-46: A custom element definition was found in a folder that should not contain element definitions. If you want to define a custom element, either place it in ui/components/ or in a components sub-folder of a panel. E.g. panels/elements/components/.

(rulesdir/enforce-custom-element-definitions-location)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/version/VersionBanner.ts at line 13, the custom
element is defined outside of the required components/ folder; move the file to
front_end/panels/ai_chat/ui/components/VersionBanner.ts, update all imports
across the codebase to reference the new path, and modify the panel's BUILD.gn
to reflect the new source location so the linter and build system recognize the
component in ui/components/.

Comment on lines +21 to +26
get info(): VersionInfo | null { return this.#info; }
set info(value: VersionInfo | null) { this.#info = value; this.#render(); }
get dismissed(): boolean { return this.#dismissed; }
set dismissed(value: boolean) { this.#dismissed = value; this.#render(); }

connectedCallback(): void { this.#render(); }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Align with lint: view method, explicit return types, and avoid direct innerHTML

  • Rename #render to render (lint: no-lit-render-outside-of-view).
  • Add return type on #dismiss.
  • Clear via Lit.render(html``) rather than innerHTML (XSS rule).
  • Update setters and connectedCallback to call render().
   get info(): VersionInfo | null { return this.#info; }
-  set info(value: VersionInfo | null) { this.#info = value; this.#render(); }
+  set info(value: VersionInfo | null) { this.#info = value; this.render(); }
   get dismissed(): boolean { return this.#dismissed; }
-  set dismissed(value: boolean) { this.#dismissed = value; this.#render(); }
+  set dismissed(value: boolean) { this.#dismissed = value; this.render(); }

-  connectedCallback(): void { this.#render(); }
+  connectedCallback(): void { this.render(); }

-  #dismiss = () => { this.dispatchEvent(new CustomEvent('dismiss', {bubbles: true})); };
+  #dismiss: () => void = () => { this.dispatchEvent(new CustomEvent('dismiss', {bubbles: true})); };

-  #render(): void {
-    if (!this.#info || !this.#info.isUpdateAvailable || this.#dismissed) { this.#shadow.innerHTML = ''; return; }
+  render(): void {
+    if (!this.#info || !this.#info.isUpdateAvailable || this.#dismissed) {
+      Lit.render(html``, this.#shadow, {host: this});
+      return;
+    }
     const info = this.#info;
     Lit.render(html`
       <style>
-        :host { position: relative; z-index: 9999; }
-        .banner { display: flex; align-items: center; justify-content: space-between; padding: 8px 12px; border-radius: 6px; background: var(--sys-color-surface-variant); color: var(--sys-color-on-surface); margin: 8px 0; position: relative; z-index: 9999; }
+        :host { position: relative; z-index: 9999; }
+        .banner { display: flex; align-items: center; justify-content: space-between; padding: 8px 12px; border-radius: 6px; background: var(--sys-color-surface-variant); color: var(--sys-color-on-surface); margin: 8px 0; position: relative; }

Also applies to: 30-45

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/version/VersionBanner.ts around lines 21-26 (and
also apply same changes to lines 30-45), the class uses a private #render
method, omits an explicit return type for #dismiss, and clears DOM via innerHTML
which violates lint/XSS rules; rename #render to a public render method and
update all internal calls (setters and connectedCallback) to call render(), add
an explicit return type to the #dismiss method (e.g., : void), and replace any
direct innerHTML clearing with Lit.render(html``, container) to safely
update/clear the element content, ensuring imports are adjusted if needed.

Comment on lines +33 to +44
Lit.render(html`
<style>
:host { position: relative; z-index: 9999; }
.banner { display: flex; align-items: center; justify-content: space-between; padding: 8px 12px; border-radius: 6px; background: var(--sys-color-surface-variant); color: var(--sys-color-on-surface); margin: 8px 0; position: relative; z-index: 9999; }
.link { color: var(--sys-color-primary); text-decoration: none; margin-left: 8px; }
button.dismiss { border: none; background: transparent; color: var(--sys-color-on-surface); cursor: pointer; font-size: 14px; }
</style>
<div class="banner">
<div>🎉 New version ${info.latestVersion} is available! <a class="link" href=${info.releaseUrl} target="_blank" rel="noopener noreferrer">View Release</a></div>
<button class="dismiss" title="Dismiss" @click=${this.#dismiss}></button>
</div>
`, this.#shadow, {host: this});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Use XLink instead of raw (ESLint: no-a-tags-in-lit)

DevTools forbids anchors in lit templates. Replace with the project XLink component.

Proposed markup change (import path may vary—see script below to find it):

-        <div>🎉 New version ${info.latestVersion} is available! <a class="link" href=${info.releaseUrl} target="_blank" rel="noopener noreferrer">View Release</a></div>
+        <div>
+          🎉 New version ${info.latestVersion} is available!
+          <x-link class="link" href=${info.releaseUrl} external>View Release</x-link>
+        </div>

Add an accessible label for the dismiss control:

-        <button class="dismiss" title="Dismiss" @click=${this.#dismiss}>✕</button>
+        <button class="dismiss" title="Dismiss" aria-label="Dismiss version banner" @click=${this.#dismiss}>✕</button>

Run to locate XLink and confirm the correct import/usage:


🏁 Script executed:

#!/bin/bash
# Find XLink component and usage in the repo to import correctly and satisfy lint.
fd -HI --full-path 'XLink.ts'
rg -nP -C2 "(customElement\\(|define\\().*['\"]x-link['\"]" --type ts

Length of output: 408


🏁 Script executed:

rg -nP -C3 "export class XLink" front_end/ui/legacy/XLink.ts

Length of output: 320


Use <x-link> instead of <a> and add aria-label to dismiss button
Import the XLink component at the top of VersionBanner.ts:

import '../../../../ui/legacy/XLink.js';

Replace the anchor tag:

- 🎉 New version ${info.latestVersion} is available! <a class="link" href=${info.releaseUrl} target="_blank" rel="noopener noreferrer">View Release</a>
+ 🎉 New version ${info.latestVersion} is available!
+ <x-link class="link" href=${info.releaseUrl} external>View Release</x-link>

Add an accessible label on the dismiss control:

- <button class="dismiss" title="Dismiss" @click=${this.#dismiss}>✕</button>
+ <button class="dismiss" title="Dismiss" aria-label="Dismiss version banner" @click=${this.#dismiss}>✕</button>
🧰 Tools
🪛 ESLint

[error] 33-44: Lit render calls should be inside of a view function

(rulesdir/no-lit-render-outside-of-view)


[error] 33-44: Found an anchor element in a lit template. Use XLink.ts instead.

(rulesdir/no-a-tags-in-lit)


[error] 33-44: Found an anchor element in a lit template. Use XLink.ts instead.

(rulesdir/no-a-tags-in-lit)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/version/VersionBanner.ts around lines 33 to 44,
replace the native anchor with the project's XLink component and add an
accessible label to the dismiss button: import '../../../../ui/legacy/XLink.js'
at the top of the file, replace the <a> element used for the release link with
an <x-link> element preserving the href, target and rel attributes and the link
text, and add an aria-label (e.g., "Dismiss version banner") to the
button.dismiss element so the control is accessible to screen readers.

Copy link

claude bot commented Sep 6, 2025

Claude finished @tysonthomas9's task —— View job


Claude completed the code review

📋 Comprehensive PR Review: Agent Framework Update

This is an impressive and substantial refactoring that modernizes the agent framework with significant architectural improvements. Here's my detailed assessment:


Strengths & Excellent Design Decisions

🏗️ Architecture & Design

  • Event-driven architecture: The new AgentRunnerEventBus provides clean separation between agent execution and UI updates
  • Provider-aware execution: LLM provider context is properly propagated through the execution chain
  • Message sanitization: Smart capability-aware filtering for vision vs non-vision models
  • Modular UI components: Well-separated concerns with LiveAgentSessionComponent, ToolCallComponent, etc.
  • Comprehensive tool context: The new LLMContext interface enables tools to make provider-aware decisions

🔒 Security Improvements

  • Input sanitization: MessageSanitizer.ts properly filters image content for non-vision models
  • Deep cloning: Prevents mutation of original message objects
  • Content filtering: Removes sensitive fields like agentSession and imageData before sending to LLM

🧪 Test Coverage

  • 19 test files added with comprehensive coverage
  • Edge case testing: Covers sanitization, tool execution, UI interactions, session management
  • Integration tests: Full workflow testing in AgentRunner.run.flows.test.ts
  • Security test cases: MessageSanitizer tests include proper filtering scenarios

⚠️ Critical Issues & Security Concerns

🛡️ XSS Vulnerabilities

  • LiveAgentSessionComponent.ts:122: Direct innerHTML assignment with unsanitized content from uiConfig.displayName and session.agentReasoning
  • VersionBanner.ts:33-44: HTML injection vulnerability in template
  • OAuthConnectPanel.ts:31: Direct DOM manipulation

Recommendation: Replace innerHTML usage with Lit templating system or implement proper HTML escaping:

// Replace this dangerous pattern:
this.shadow.innerHTML = `<div>${uiConfig.displayName}</div>`;

// With safe Lit templating:
Lit.render(html`<div>${Lit.Directives.unsafeHTML(this.escapeHtml(uiConfig.displayName))}</div>`, this.shadow);

💾 Data Exposure

  • AIChatPanel.ts:49-72: Storage monitoring logs API keys and secrets to debug logs
  • AgentRunner.ts:950: Tool arguments containing PII logged in plaintext
  • AgentRunner.ts:161-184: Shallow sanitization may miss nested secrets

Recommendation: Implement comprehensive secret redaction:

private redactSecrets(obj: any): any {
  const sensitiveKeys = /api_key|apikey|token|secret|password|auth/i;
  // Deep traversal with secret masking
}

🚀 Performance Considerations

⚡ Optimizations Needed

  • MessageSanitizer.ts:11: JSON.parse(JSON.stringify()) is inefficient for deep cloning large objects
  • Memory leaks: UI component maps (toolComponents, childComponents) may accumulate without proper cleanup
  • Double rendering: Pre-rendering tokens in MarkdownRenderers.ts:76 causes duplicate processing

Recommendations:

// Replace JSON deep clone with structured clone API
function deepClone<T>(obj: T): T {
  return structuredClone(obj);
}

// Add proper component cleanup
disconnectedCallback(): void {
  this.toolComponents.clear();
  this.childComponents.clear();
}

🐛 Notable Bugs

🔧 Logic Issues

  1. AgentRunner.ts:1221: Duplicate child session insertion causes UI duplication
  2. MessageCombiner.ts:99: Accesses non-existent error field on ToolResultMessage
  3. ConfigurableGraph.ts:59: Non-null assertions ! can cause runtime crashes
  4. VersionBanner.ts:26: Missing return types on methods

🎯 Type Safety

  • Excessive as any usage throughout codebase (15+ instances)
  • Missing imports in test files (assert from chai)
  • Inconsistent error handling patterns

📊 Code Quality Assessment

💯 Best Practices Followed:

  • Clear separation of concerns
  • Consistent TypeScript interfaces
  • Proper event handling patterns
  • Good logging practices (when not exposing secrets)

🔧 Areas for Improvement:

  • Error Handling: Many empty catch {} blocks mask important errors
  • Type Safety: Replace any types with proper interfaces
  • Documentation: Complex workflows need more inline documentation
  • Accessibility: Missing ARIA attributes on interactive elements

🎯 Recommendations by Priority

🔥 High Priority (Security)

  1. Fix XSS vulnerabilities in UI components
  2. Implement proper secret redaction in logging
  3. Add input validation for all user-facing APIs

⚡ Medium Priority (Stability)

  1. Replace non-null assertions with proper validation
  2. Fix duplicate child session bug
  3. Add proper component lifecycle management

🚀 Low Priority (Quality)

  1. Improve error handling patterns
  2. Add comprehensive type safety
  3. Enhance accessibility features

🏆 Overall Assessment

This PR represents a significant architectural improvement with excellent design principles. The event-driven approach, provider-aware execution, and comprehensive test coverage demonstrate high-quality engineering.

Rating: ⭐⭐⭐⭐☆ (4/5 stars)

Recommendation: Approve with security fixes required - Address the XSS vulnerabilities and secret logging issues before merging. The architectural improvements far outweigh the issues, which are primarily security-focused and can be resolved quickly.

The foundation laid here will enable much more flexible and secure agent operations going forward. Excellent work on the overall design!


@tysonthomas9 tysonthomas9 merged commit d0ce4d2 into main Sep 6, 2025
1 of 2 checks passed
tysonthomas9 added a commit that referenced this pull request Sep 28, 2025
- New Features
  - Introduced rich AI Chat UI: message list, input bar, model selector, tool call/result cards, agent session header, and live session timeline with nesting.
  - Added structured response viewing with auto-open and “View Full Report.”
  - OAuth connect panel and version update banner.
- Improvements
  - Real-time agent progress events (session/tool start, completion, child sessions).
  - Provider-aware, vision-capable message handling and tool executions.
  - Enhanced Markdown rendering (TOC, CSS blocks).
  - Centralized scroll-to-bottom behavior; wider input and better autosizing.
- Bug Fixes
  - Normalized tool call data and content for Groq models.
  - Input clears after send (Enter or button).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant